Adding slack integration for posting to channel on applies. Fixes #179 #180

nicholas-wu-hs · 2017-11-02T21:14:42Z

Tackling #179, first iteration using slack-token and slack-channel in the config.
If specified, a Slack Client will be created and attached to ApplyExecutor. It's used to post messages to slack-channel on applies (both success and fail).

Using https://github.com/nlopes/slack for slack client.

Will probably need a lot of refactoring/restructuring, then eventually tests and docs?
Also, was I supposed to add the dependencies into the repo?

nicholas-wu-hs · 2017-11-02T21:20:05Z

server/server.go

@@ -61,6 +64,14 @@ func NewServer(config Config) (*Server, error) {
 		return nil, err
 	}
 	githubStatus := &events.GithubStatus{Client: githubClient}
+	// nil slackClient unless token and channel was specified
+	var slackClient slack.Client
+	if config.SlackToken != "" && config.SlackChannel != "" {


Only attempt to create a slack client if both token and channel are supplied.
If we decide to only use a token and channel, I plan to add a warning for when the user supplies ONLY one item. But the schema will probably be changed after discussion in #179 and this'll be irrelevant.

In general it's dangerous to use nil for something like this because it's not obvious that my slack client will be nil just because somewhere up the chain depending on the config. On the other hand, you can't initialize a slack client if you don't have the config :D.

I would create a NoopSlackClient that implements the interface but doesn't do anything and use it here if there is no slack config.

lkysow · 2017-11-02T23:06:46Z

server/events/apply_executor.go

@@ -106,3 +114,19 @@ func (a *ApplyExecutor) apply(ctx *CommandContext, repoDir string, plan models.P

 	return ProjectResult{ApplySuccess: output}
 }
+
+func createSlackMessage(ctx *CommandContext, success bool) string {


this functionality doesn't belong in the apply executor since it's all about applying, not creating slack messages. Put this in the slack client.

don't use package functions like this, instead add it to the struct.
func (a *ApplyExecutor) createSlackMessage

If you don't do this then anyone can call this method from anywhere in this package which is weird, you really only want it called from the object

lkysow · 2017-11-02T23:08:22Z

server/events/apply_executor.go

+		status = ":x:"
+	}
+
+	return fmt.Sprintf("%s *%s* %s in <%s|%s>.",


can you add a screenshot so we can see what this looks like?

Here it is for using the emojis!

Also had them as text at one point:

And as attachments but they're chunkier:

lkysow · 2017-11-02T23:09:36Z

server/events/slack/slack_client.go

+	}
+
+	// https://api.slack.com/faq
+	// 'How do I find a channel's ID if I only have its #name?'


I don't think you need this. It says you can just use the name: https://api.slack.com/methods/chat.postMessage#channels

With chat.postMessage you can use a channel name, but if the channel doesn't exist, it doesn't send.

I was thinking of making sure the channel from config exists on the creation of the slack client and failing early if it doesn't, rather than failing at chat.postMessage or failing silently.

So to do that, I looked at https://api.slack.com/methods/channels.info which requires an ID. Alternatively, I can do a chat.postMessage and see if it succeeds but that would post a message? Or there might be other ways!

I like the idea of failing when Atlantis server starts if that channel doesn't exist. Failing early is usually better so good call.

lkysow · 2017-11-02T23:11:42Z

server/events/slack/slack_client.go

+		}
+	}
+
+	return nil, errors.New("channel_not_found")


errors should be human readable so you could have just had "channel doesn't exist"

lkysow · 2017-11-02T23:16:46Z

server/server.go

@@ -61,6 +64,14 @@ func NewServer(config Config) (*Server, error) {
 		return nil, err
 	}
 	githubStatus := &events.GithubStatus{Client: githubClient}
+	// nil slackClient unless token and channel was specified
+	var slackClient slack.Client
+	if config.SlackToken != "" && config.SlackChannel != "" {


In general it's dangerous to use nil for something like this because it's not obvious that my slack client will be nil just because somewhere up the chain depending on the config. On the other hand, you can't initialize a slack client if you don't have the config :D.

I would create a NoopSlackClient that implements the interface but doesn't do anything and use it here if there is no slack config.

This reverts commit 3be7f8c.

nickwu241 · 2017-11-04T22:23:10Z

Thanks for the awesome review and feedback!

Restructured to be more generic and handle Webhook config schema discussed at Slack integration on apply #179
Fixed errors to be human readable
Refactored slack related items to the server/events/slack
Reverted 3be7f8c (adding vendor deps) for now so it's easier to review

TODO:

Proper error handling and messages
Check if slack channel exists on server startup and error early if it doesn't
?Default workspace-regex to ".*" if not provided
?Pass command success/failure status down to HookExecutors
Add tests

nickwu241 · 2017-11-04T22:33:03Z

server/server.go

+	for _, webhook := range config.Webhooks {
+		hookExecutor, err := hookExecutorBuilder.build(webhook)
+		if err != nil {
+			// TODO: check if it was because of uninitialized slack client


If slack-token wasn't provided in the config, hookExecutorBuilder would have a nil slack.Client. This will cause an error when building slack webhooks, then Atlantis can error saying the user needs to provide a slack-token.

But this doesn't seem too clean, any suggestions? Using a NoopClient would work, but if we can error early and tell the user what's wrong, I feel like that's the way to go!

In my suggestions I have a WebhooksManager that has a []WebhookSender. When it gets constructed with no webhooks then it just has an empty array and so calling WebhooksManager.Send() just does nothing. That's how I handled it.

When it's running through the webhooks in its constructor, it returns an error if a slack webhook was specified but there was no slack-token: https://github.com/nicholas-wu-hs/atlantis/pull/1/files?diff=unified#diff-0250d5b43682815402ad30ce1b068cdcR55

nickwu241 · 2017-11-04T22:36:44Z

server/events/apply_executor.go

+		matched, err := regexp.MatchString(workspaceRegex, ctx.Command.Environment)
+		if err != nil {
+			// log the regexp error but let's continue with the apply
+			ctx.Log.Debug(err.Error())


Maybe we can verify the regex during the config processing stage?
Does a debug message make sense here? Or should it be an error :O

Maybe we can verify the regex during the config processing stage?

exactly regexp.Compile

lkysow · 2017-11-06T04:40:59Z

I'll add specific comments to your code but in general here's the changes I think need to be made and why:

lets move everything under an events/webhooks package. I don't think slack needs its own package at the moment and then all our code can be under that package cleanly including hook_executor
I want the code in ApplyExecutor to be as clean as possible and not need to know anything about webhooks. The ideal call would be just be

	a.WebhooksManager.Send(webhookResult)

constructing all the webhooks clients and verifying the regex, etc. should happen inside the WebhooksManager constructor. So in server/server.go it should just look like

	webhookManager, err := webhooks.NewWebhooksManager(webhooksConfig, config.SlackToken)
	if err != nil {
		return nil, errors.Wrap(err, "initializing webhooks")
	}

the interfaces I'd like to see are
- a WebhooksSender that handles sending out multiple webhooks. This is what the ApplyExecutor has a reference to.

type WebhooksSender interface {
	Send(log *logging.SimpleLogger, result ApplyResult)
}

a WebhookSender that handles sending out a single webhook. This can either be implemented by the SlackSender or a GenericSender. The WebhooksSender will have a reference to multiple WebhookSender's. These will decide based on the environment whether to send out their webhooks or not.

type WebhookSender interface {
	Send(ApplyResult) error
}

I've messed around with the code and created a pr back into your branch for what I'd like to see: nicholas-wu-hs#1. Feel free to just use that as a guide if you want to code it out yourself.

lkysow · 2017-11-06T04:42:37Z

server/server.go

+	Channel string `mapstructure:"channel"`
+}
+
+type HookExecutorBuilder struct {


I would all of this code out of the server package and into events/webhooks. Ask yourself if building a HookExecutor should be the responsibility of the server class

Good point, I thought about this too.
Previously, I tried to pass the the config structs from the server package to the server/events/webhooks code and ran into circular import problems, googled a bit for more info but found no clear solution.

In general, does a .go file import packages from parent directories?

no it doesn't because of those circular import errors you ran into. You can either create a separate package (like models) or you only reference from parent to child, never from child to parent

Got it, thank you 👍

lkysow · 2017-11-06T04:43:07Z

server/server.go

+	Slack slack.Client
+}
+
+func (builder *HookExecutorBuilder) build(webhook Webhook) (events.HookExecutor, error) {


pointer receivers should just be the first letter of the struct, in this case h *HookExecutorBuilder.

Got it now! Thanks, will do this for all future code!

lkysow · 2017-11-06T04:45:43Z

server/server.go

+	for _, webhook := range config.Webhooks {
+		hookExecutor, err := hookExecutorBuilder.build(webhook)
+		if err != nil {
+			// TODO: check if it was because of uninitialized slack client


In my suggestions I have a WebhooksManager that has a []WebhookSender. When it gets constructed with no webhooks then it just has an empty array and so calling WebhooksManager.Send() just does nothing. That's how I handled it.

When it's running through the webhooks in its constructor, it returns an error if a slack webhook was specified but there was no slack-token: https://github.com/nicholas-wu-hs/atlantis/pull/1/files?diff=unified#diff-0250d5b43682815402ad30ce1b068cdcR55

lkysow · 2017-11-06T04:46:30Z

server/server.go

@@ -86,6 +142,7 @@ func NewServer(config Config) (*Server, error) {
 	}
 	applyExecutor := &events.ApplyExecutor{
 		Github:            githubClient,
+		WSRegexToHook:     applyExecutorRegexToHook,


here's where I would have liked to see something like a WebhooksManager so the ApplyExecutor only needs to call a single line of code to send webhooks

lkysow · 2017-11-06T04:48:33Z

server/server.go

+	Webhooks            []Webhook `mapstructure:"webhooks"`
+}
+
+type Webhook struct {


I'd call this WebhookConfig since it's no an actual webhook, just config for it.

In my suggestions I don't pass this in directly to the WebhooksManager. This is because I want the config that users need in their config files to be decoupled from the WebhooksManager. This will prevent us from accidentally changing the name of a config option in the WebhooksManager code and invalidating everyone's config files :) We'll have to explicitly do the mapping: https://github.com/nicholas-wu-hs/atlantis/pull/1/files?diff=unified#diff-91bbeda7eb98a7adc57b9e47e2cf5c2bR72

lkysow · 2017-11-06T04:49:38Z

server/events/apply_executor.go

+
+	// execute apply webhooks
+	for workspaceRegex, hookExecutor := range a.WSRegexToHook {
+		matched, err := regexp.MatchString(workspaceRegex, ctx.Command.Environment)


if you had constructed the regex during the construction of the slack webhook sender then here you could have a reference to *regexp.Regexp and call .MatchString() which won't return an error.

Awesome, thank you!

lkysow · 2017-11-06T04:51:24Z

server/events/slack/slack_client.go

+	client *slack.Client
+}
+
+type SlackMessageTemplate struct {


good call to have this as a struct rather than multiple params

lkysow · 2017-11-06T04:51:56Z

server/events/slack/slack_client.go

+	}, nil
+}
+
+func (s *ConcreteClient) PostMessage(channel string, template SlackMessageTemplate) (string, error) {


here you should have (c *ConcreteClient)

lkysow · 2017-11-06T04:52:17Z

server/events/slack/slack_client.go

+	return timestamp, err
+}
+
+func (s *ConcreteClient) createMessage(template SlackMessageTemplate) string {


I want to do the attachment style message that you showed earlier. I think that looks best.

Agreed that it looks nicer, one downside is that it takes a bit more room (1 attachment is at min ~3 lines), I'll play around a bit more with the message style.

yeah it takes up more room but I think it's worth it because it allows the information to be shown in a more consumable manner

lkysow · 2017-11-06T04:56:56Z

server/events/apply_executor.go

+		matched, err := regexp.MatchString(workspaceRegex, ctx.Command.Environment)
+		if err != nil {
+			// log the regexp error but let's continue with the apply
+			ctx.Log.Debug(err.Error())


Maybe we can verify the regex during the config processing stage?

exactly regexp.Compile

Refactor webhooks

nicholas-wu-hs · 2017-11-10T18:09:18Z

Still working on the tests, looking into using pegomock, I'll let you know when this is ready for another review :)

So this is another shot at using slack attachments, what do you think?

is Environment important enough to keep as its own field?
do we need a Status field? it's encoded in the colors but might not be clear
if we want the attachment to be smaller, we can do something like Applied <ENV> in <REPO> by <USER>. or <USER> applied <ENV> in <REPO>.

lkysow · 2017-11-12T18:45:23Z

server/events/apply_executor.go

@@ -92,6 +94,16 @@ func (a *ApplyExecutor) apply(ctx *CommandContext, repoDir string, plan models.P
 	env := ctx.Command.Environment
 	tfApplyCmd := append(append(append([]string{"apply", "-no-color"}, applyExtraArgs...), ctx.Command.Flags...), plan.LocalPath)
 	output, err := a.Terraform.RunCommandWithVersion(ctx.Log, absolutePath, tfApplyCmd, terraformVersion, env)
+
+	// Send webhooks.


In general comments should be used to

explain why a piece of code is doing what it is

explain what a large section of code is doing if it's not clear from just reading it

For 2. we should strive to write code that removes the need to comment it because it's so clear.

In this case, you've done that, it's pretty obvious what a.Webhooks.Send is doing and so the comment Send webhooks. doesn't tell me any more information.

The problem with comments is that they're a form of duplication and they're harder to keep up to date. If I rename Send() to Queue() then my code won't compile if I don't change the function everywhere, but your comment might get left behind and fall out of date.

tl;dr: remove this comment, it adds nothing

Thanks for the insight, I agree with the view on what comments should be and developers should strive to write self-documenting code -- I'll will remove the comment :P

lkysow · 2017-11-12T18:46:53Z

server/events/webhooks/slack.go

+		return nil, errors.Wrap(err, "testing slack authentication")
+	}
+
+	// Make sure the slack channel exists.


This is a good comment. I'd add
Make sure the slack channel exists so we can error early.

lkysow · 2017-11-12T18:49:33Z

server/events/webhooks/slack.go

+func (s *SlackWebhook) createAttachments(result ApplyResult) []slack.Attachment {
+	var color string
+	if result.Success {
+		color = "good"


make these constants at the top of file underneath the import section:

const successColour = "good" const failureColour = "danger"

(also color => colour because we're canadian :D 🇨🇦 )

lkysow · 2017-11-12T18:50:07Z

server/events/webhooks/slack_test.go

+package webhooks_test
+
+// todo: actually test
+// purposefully empty to trigger coverage report


nice work figuring this out

lkysow · 2017-11-12T18:55:11Z

server/events/webhooks/webhooks_test.go

+	configs := validConfigs()
+	configs[0].WorkspaceRegex = invalidRegex
+	_, err := webhooks.NewWebhooksManager(configs, validToken)
+	Assert(t, strings.Contains(err.Error(), "error parsing regexp"), "expected err")


if err is nil then calling err.Error() will cause a panic in the test

e.Error() panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1094fa6]

This is fine right now because your code is working but if someone breaks the code then the tests will be hard to figure out what's wrong because they will just have a panic. So I'd add an extra test here

Assert(t, err != nil, "expected error")

And then if err is not nil you'll get an easier to debug failure.

That makes sense, now I know and will nil check errors in the tests now & for the future, ty!

lkysow · 2017-11-12T19:00:50Z

server/events/webhooks/webhooks_test.go

+	configs := validConfigs()
+	configs[0].Kind = unsupportedKind
+	_, err := webhooks.NewWebhooksManager(configs, validToken)
+	Assert(t, strings.Contains(err.Error(), "kind"), "expected err")


use Equals and check the whole error string

lkysow · 2017-11-12T19:02:38Z

server/events/webhooks/webhooks_test.go

+	Channel:        validChannel,
+}
+
+func validConfigs() []webhooks.Config {


what is this doing? Why are you appending an empty slice to validConfig? Why not just have

var validConfig = []webhooks.Config{ {Event: ...} }

What I wanted to do was get a fresh copy of the valid configs for each test since I modify it in some tests.
So I think after having validConfig as a variable per your example, I can do config := validConfig in my tests to copy it.
Thanks!

I left it as a function because I want new copies of the valid configs in each test, but I got rid of the appending.

lkysow · 2017-11-12T19:03:58Z

server/events/webhooks/webhooks_test.go

+func TestNewWebhooksManager_NoSlackToken(t *testing.T) {
+	emptyToken := ""
+	_, err := webhooks.NewWebhooksManager(validConfigs(), emptyToken)
+	Assert(t, strings.Contains(err.Error(), "slack-token must be set"), "expected err")


we should also test that if there are no configs then you don't need a slack-token

lkysow · 2017-11-12T19:04:31Z

server/events/webhooks/webhooks_test.go

+	// todo: failing because not a valid token and channel
+	m, err := webhooks.NewWebhooksManager(validConfigs(), validToken)
+	Ok(t, err)
+	Assert(t, m != nil, "mangager shouldn't be nil")


mangager => manager

thanks for the catch!

lkysow · 2017-11-12T19:08:01Z

server/server.go

 }

 func NewServer(config Config) (*Server, error) {
+	var webhooksConfig []webhooks.Config
+	for _, c := range config.Webhooks {
+		webhooksConfig = append(webhooksConfig, webhooks.Config{Channel: c.Channel, Event: c.Event, Kind: c.Kind, WorkspaceRegex: c.WorkspaceRegex})


let's use a multiline config for the struct here since this line is getting long

nicholas-wu-hs · 2017-11-14T20:18:31Z

server/events/webhooks/mocks/mock_slack.go

+	return ret0, ret1
+}
+
+func (mock *MockSlackClient) PostMessage(channel string, result ApplyResult) error {


the generated code from pegomock doesn't compile because it doesn't know what ApplyResult is. (same for mock_webhooks.go)
I tried adding in the package manually but then got errors about pegomock.GetGenericMockFrom(mock).Invoke return type is incompatible.
Still figuring this out!

nicholas-wu-hs · 2017-11-14T21:16:32Z

Still figuring out mocks and adding more tests to slack_test.go, but so far I've

changed color => colour, move red, green to constants
fixed mangager => manager
removed redundant comment
added log message for most tests (still WIP)
added nil checks before err.Error() in tests
tested for whole strings for event and kind
refactored slack.go so it can be mocked

I'll ask for a review after I'm done but wouldn't mind another review :)

lkysow · 2017-11-14T21:55:50Z

server/events/webhooks/slack.go

+	failureColour = "danger"
+)
+
+//go:generate pegomock generate --use-experimental-model-gen --package mocks -o mocks/mock_slack.go slack.go


change slack.go to SlackClient. Follow method 2. here: https://github.com/petergtz/pegomock#generating-mocks

Wow, thank you ahahaha, that fixed the can't find ApplyResult
Now I'm looking into

mocks/mock_slack.go:50:9: no new variables on left side of := mocks/mock_slack.go:50:9: cannot use pegomock.GetGenericMockFrom(mock).Invoke("PostMessage", params, []reflect.Type literal) (type pegomock.ReturnValues) as type webhooks.ApplyResult in assignment mocks/mock_slack.go:52:8: invalid argument result (type webhooks.ApplyResult) for len mocks/mock_slack.go:53:12: invalid operation: result[0] (type webhooks.ApplyResult does not support indexing) mocks/mock_slack.go:54:17: invalid operation: result[0] (type webhooks.ApplyResult does not support indexing)

as you mentioned, this was due to variable name clashing so I changed result to applyResult on the interface, thanks for catching this :)

lkysow · 2017-11-14T21:56:37Z

server/events/webhooks/webhooks.go

+const SlackKind = "slack"
+const ApplyEvent = "apply"
+
+//go:generate pegomock generate --use-experimental-model-gen --package mocks -o mocks/mock_webhooks.go webhooks.go


change webhooks.go to WebhooksSender

nicholas-wu-hs · 2017-11-15T02:38:06Z

fixed the mocks by generating from interfaces instead of files (per your feedback)
modified tests to use mock slack client
added tests to slack_test.go, all tests are passing

lkysow · 2017-11-15T18:22:04Z

Your commit isn't passing gometalint.

# gotype and gotypex are disabled because they don't pass on CI and https://github.com/alecthomas/gometalinter/issues/206
# gocyclo is temporarily disabled because we don't pass it right now
# golint is temporarily disabled because we need to add comments everywhere first
gometalinter --disable gotype --disable gotypex --disable=gocyclo --disable golint --enable=megacheck --enable=unparam --deadline=120s --vendor -t --line-length=120 $(find . -type f -name '*.go' ! -path "./vendor/*" ! -path "./server/static/bindata_assetfs.go" ! -path "**/mocks/*" | xargs -I '{}' dirname '{}' | sort -u)
server/events/webhooks/slack_test.go:29:11:warning: error return value not checked (hook.Send(result)) (errcheck)
Makefile:51: recipe for target 'gometalint' failed

You can run the make target locally to test.

nicholas-wu-hs · 2017-11-15T18:26:53Z

I see! I think I know the problem from the error message, will add in the fixes.
EDIT: fixed it in b17eec2, just found out you can click details to see what failed haha

Should I add in the vendor dependencies (i.e. the slack library) as part of this pull request?

lkysow · 2017-11-15T18:25:24Z

server/events/webhooks/slack.go

+	failureColour = "danger"
+)
+
+//go:generate pegomock generate --use-experimental-model-gen --package mocks -o mocks/mock_slack.go SlackClient


use mocks/mocks_slack_client.go to match the interface name rather than the name of this file. That way, if we generate a mock for more than one interface in this file, it won't delete the previously generated file (which would have the same name)

lkysow · 2017-11-15T18:26:05Z

server/events/webhooks/slack.go

+	PostMessage(channel string, applyResult ApplyResult) error
+}
+
+type ConcreteSlackClient struct {


I've decided to go with Default for the actual implementations of interfaces, so call this DefaultSlackClient. Still need to fix a bunch of the other stuff but may as well start here.

lkysow · 2017-11-15T18:27:01Z

server/events/webhooks/slack.go

+	}
+}
+
+func NewSlack(r *regexp.Regexp, channel string, client SlackClient) (*SlackWebhook, error) {


split the slack client and webhook into two files slack_client.go and slack.go

lkysow · 2017-11-15T18:27:31Z

server/events/webhooks/slack.go

+
+type SlackClient interface {
+	AuthTest() error
+	ChannelExist(channelName string) (bool, error)


call this ChannelExists so it reads properly

lkysow · 2017-11-15T18:28:59Z

server/events/webhooks/slack_test.go

+	RegisterMockTestingT(t)
+	client := mocks.NewMockSlackClient()
+	regex, err := regexp.Compile(".*")
+	Ok(t, err)


lkysow · 2017-11-15T18:30:57Z

server/events/webhooks/slack_test.go

+		Environment: "production",
+	}
+	err = hook.Send(result)
+	Ok(t, err)


have you tried commenting out the line where you check if the regex matches (in the actual implementation) and seeing if this test passes (I think it will). You need to check that PostMessage was never called. I think you can do something easy like client.VerifyWasCalled(Never()).PostMessage(...) to ensure it is never called.

Yeah, you're correct, the test passes commenting out the regex match.
client.VerifyWasCalled(Never()).PostMessage(...) works and the test fails if i comment out the regex match! ty ty

Also renamed Concrete -> Default, Exist -> Exists

Slack integration

nicholas-wu-hs · 2017-11-17T02:46:33Z

Finished writing the tests

lkysow · 2017-11-17T04:13:49Z

Merged in #199!

nicholas-wu-hs added 3 commits November 2, 2017 13:48

Add server/events/slack/slack_client.go

d5d8e12

Parse slack config, add slack client to apply_executor

5491a30

Vendor using dep

3be7f8c

nicholas-wu-hs commented Nov 2, 2017

View reviewed changes

lkysow reviewed Nov 2, 2017

View reviewed changes

nicholas-wu-hs added 2 commits November 4, 2017 14:49

Add hook_executor for webhook integration, WIP

3976d46

Revert "Vendor using dep"

8c1dc0f

This reverts commit 3be7f8c.

nickwu241 reviewed Nov 4, 2017

View reviewed changes

Refactor webhooks

46d9f44

lkysow reviewed Nov 6, 2017

View reviewed changes

nicholas-wu-hs and others added 4 commits November 7, 2017 20:09

Merge pull request #1 from hootsuite/slack-webhooks-lkysow

f94cede

Refactor webhooks

Add test channel exist

e35fd72

Use attachment instead of text

4cd775b

Create first round of webhooks tests

ed1d8cd

lkysow reviewed Nov 12, 2017

View reviewed changes

Refactor and mock slack.go, fix reviewed changes

4e57157

nicholas-wu-hs force-pushed the slack-integration branch from 197aa3f to 4e57157 Compare November 14, 2017 20:15

nicholas-wu-hs commented Nov 14, 2017

View reviewed changes

lkysow reviewed Nov 14, 2017

View reviewed changes

Fix and regen mocks, finish slack webhooks tests

0d39196

Fix slack_test.go gometalint warning of error not checked

b17eec2

lkysow reviewed Nov 16, 2017

View reviewed changes

Split slack.go into slack_client.go, add new mocks & tests

66a6f6f

Also renamed Concrete -> Default, Exist -> Exists

nicholas-wu-hs force-pushed the slack-integration branch from 1cc684a to 66a6f6f Compare November 16, 2017 07:59

Luke Kysow and others added 3 commits November 16, 2017 14:34

Merge remote-tracking branch 'origin/master' into slack-integration

9b680e5

Start on slack changes

70ad95f

Merge pull request #2 from hootsuite/slack-integration

e1d059e

Slack integration

nicholas-wu-hs force-pushed the slack-integration branch 3 times, most recently from 1b67e1d to 5d9ebaa Compare November 17, 2017 02:13

nicholas-wu-hs added 2 commits November 16, 2017 18:28

Add SlackWraper mocks, update slack_client_test.go

1ebc62b

Remove config.yml

54e4207

nicholas-wu-hs force-pushed the slack-integration branch from 8cc6089 to 54e4207 Compare November 17, 2017 02:29

lkysow closed this Nov 17, 2017

nicholas-wu-hs deleted the slack-integration branch November 20, 2017 21:28

Adding slack integration for posting to channel on applies. Fixes #179 #180

Adding slack integration for posting to channel on applies. Fixes #179 #180

Conversation

nicholas-wu-hs commented Nov 2, 2017 • edited Loading

nicholas-wu-hs Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

nicholas-wu-hs Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickwu241 commented Nov 4, 2017 • edited Loading

nickwu241 Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickwu241 Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

nicholas-wu-hs Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs commented Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

nicholas-wu-hs Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

nicholas-wu-hs commented Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs commented Nov 15, 2017 • edited Loading

lkysow commented Nov 15, 2017 • edited Loading

nicholas-wu-hs commented Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-wu-hs commented Nov 2, 2017 •

edited

Loading

nicholas-wu-hs Nov 2, 2017 •

edited

Loading

nicholas-wu-hs Nov 2, 2017 •

edited

Loading

nicholas-wu-hs Nov 2, 2017 •

edited

Loading

nicholas-wu-hs Nov 3, 2017 •

edited

Loading

nickwu241 commented Nov 4, 2017 •

edited

Loading

nickwu241 Nov 4, 2017 •

edited

Loading

nickwu241 Nov 4, 2017 •

edited

Loading

lkysow commented Nov 6, 2017 •

edited

Loading

nicholas-wu-hs Nov 8, 2017 •

edited

Loading

nicholas-wu-hs Nov 8, 2017 •

edited

Loading

nicholas-wu-hs Nov 8, 2017 •

edited

Loading

nicholas-wu-hs commented Nov 10, 2017 •

edited

Loading

nicholas-wu-hs Nov 14, 2017 •

edited

Loading

nicholas-wu-hs Nov 14, 2017 •

edited

Loading

nicholas-wu-hs Nov 14, 2017 •

edited

Loading

nicholas-wu-hs commented Nov 14, 2017 •

edited

Loading

nicholas-wu-hs commented Nov 15, 2017 •

edited

Loading

lkysow commented Nov 15, 2017 •

edited

Loading

nicholas-wu-hs commented Nov 15, 2017 •

edited

Loading